chore: move back to nullable forwarding. #6733
Conversation
DimitrisJim
requested review from
colin-axner,
AdityaSripal,
damiannolan,
srdtrk,
chatton,
crodriguezvega,
gjermundgaraba and
bznein
as code owners
| // Initialize empty pointer with empty forwarding information | ||
| if msg.Forwarding == nil { | ||
| msg.Forwarding = types.NewForwarding(false) | ||
| } |
There was a problem hiding this comment.
really do not like this, though
There was a problem hiding this comment.
What is this needed for?>
If it's only for the check below (and for the call to sednTransfer, maybe it can be slightly cleaner like this?
var hops []types.Hops
if msg.Forwarding != nil {
hops = msg.Forwarding.Hops
if msg.Forwarding.Unwind {
....
}
}
sequence, err := k.sendTransfer(
ctx, msg.SourcePort, msg.SourceChannel, coins, sender, msg.Receiver, msg.TimeoutHeight, msg.TimeoutTimestamp,
msg.Memo, hops)Still not super clean, but maybe marginally better?
There was a problem hiding this comment.
we'd also need to move assignment to hops after that unwind call considering it could modify it but yea, think this couild be an alt if people have preferences.
There was a problem hiding this comment.
I'd be fine with:
if msg.Forwarding != nil && msg.Forwarding.UnwindThere was a problem hiding this comment.
mainly did this because we forward hops after when calling into sendTransfer and didn't want two nil checks.
There was a problem hiding this comment.
maybe also pretty dirty, but could do something like
func (f *Forwarding) Unwind() {
if f == nil {
return false
}
return f.Unwind
}Perfectly happy with the inline check colin suggested though 👍
There was a problem hiding this comment.
hm, I definitely prefer this. Will probably do for Hops too.
lol, forgot proto generates these for us 🤦
| // forwarding information present in the message. If forwarding information is missing | ||
| // or unwinding isn't performed, we do normal validation, else, we assert that both | ||
| // fields must be empty. | ||
| func (msg MsgTransfer) validateIdentifiers() error { |
There was a problem hiding this comment.
this made more sense to me instead of having it partially in one func and partially in another.
| // Initialize empty pointer with empty forwarding information | ||
| if msg.Forwarding == nil { | ||
| msg.Forwarding = types.NewForwarding(false) | ||
| } |
There was a problem hiding this comment.
What is this needed for?>
If it's only for the check below (and for the call to sednTransfer, maybe it can be slightly cleaner like this?
var hops []types.Hops
if msg.Forwarding != nil {
hops = msg.Forwarding.Hops
if msg.Forwarding.Unwind {
....
}
}
sequence, err := k.sendTransfer(
ctx, msg.SourcePort, msg.SourceChannel, coins, sender, msg.Receiver, msg.TimeoutHeight, msg.TimeoutTimestamp,
msg.Memo, hops)Still not super clean, but maybe marginally better?
There was a problem hiding this comment.
Thank you, @DimitrisJim. I triggered a compatibility test run with this branch so that we can have fresh results tomorrow morning.
|
Seems we hit a rate limit but, on docker this time? 😆 https://github.com/cosmos/ibc-go/actions/runs/9750596978/job/26910547489#step:4:30 |
|
triggered again https://github.com/cosmos/ibc-go/actions/runs/9755361718/job/26923850921 seems we don't have an image for 7.6.0 which causes most failures. Flakes exist for ica ones, transfer ones should hopefully pass through considering they did in the run you triggered yesterday. |
Ay, I see that the release workflow failed for v7.6.0 when the release was tagged, so that would explain why there is no docker image. Not sure what the error means though... I have triggered now a docker build action for v7.6.0 |
| // Initialize empty pointer with empty forwarding information | ||
| if msg.Forwarding == nil { | ||
| msg.Forwarding = types.NewForwarding(false) | ||
| } |
There was a problem hiding this comment.
maybe also pretty dirty, but could do something like
func (f *Forwarding) Unwind() {
if f == nil {
return false
}
return f.Unwind
}Perfectly happy with the inline check colin suggested though 👍
|
Description
validation needed tweaking which resulted in not as clean of a pr as I'd wanted
closes: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.